Go: New File System Access Sinks #14064
Conversation
am0o0
changed the title
|
Hi, I just added some comments so there aren't new updates. for better review quality. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
go/ql/test/library-tests/semmle/go/security/FileSystemAccess/FileSystemAccess.ql
Fixed
Show fixed
Hide fixed
|
You also need to add a change note (details here). And the tests are failing because they aren't building correctly - for some reason it can't find the vendored dependencies. I'll look into it more next week. |
|
@owen-mc thanks for helping me to improve the quality of my pull request. how much time do usually I have to fix and submit the suggestion generally in the codeql repository? |
|
@amammad There is no great hurry. If you take more than a month I might ask if you plan to do it at some point or not. I do recommend not leaving them too long, when possible, because sometimes other changes get merged in which cause merge conflicts or require some changes to the PR. |
|
I've looked into why the test is failing. You need to make stubs for |
|
@owen-mc Hi, I've created an Afero module that supports sanitizer now! |
go/ql/test/library-tests/semmle/go/security/FileSystemAccess/main.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is looking really good. Just a few small comments. Also, I'm afraid to say that these models should be moved into different files, one for each library. There are already files for Beego, Gin and Echo. You can make ones for the others. (There is one for Fiber in the experimental folder, which you can ignore.) The tests will have to be split and moved as well.
go/ql/test/library-tests/semmle/go/security/FileSystemAccess/main.go
Outdated
Show resolved
Hide resolved
go/ql/test/library-tests/semmle/go/security/FileSystemAccess/main.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
You can put JWT.qll in go/ql/src/experimental/frameworks/JWT.qll and import it as import experimental.frameworks.JWT.
Can I just double check that the additional flow steps are valid for any query, not just this one? The way you’ve done it is fine. When this query is promoted they will probably be changed to yml models, but those can’t be used for experimental queries.
I haven't reviewed the tests yet. I will do that when I can find time.
go/ql/src/change-notes/2023-09-25-add-new-filesystemaccess-sinks.md
Outdated
Show resolved
Hide resolved
go/ql/src/change-notes/2023-09-25-add-new-filesystemaccess-sinks.md
Outdated
Show resolved
Hide resolved
...tests/semmle/go/frameworks/Afero/vendor/github.com/beego/beego/v2/server/web/context/stub.go
Outdated
Show resolved
Hide resolved
|
The following files need to be reformatted:
I'm not sure if you can see the output of CI, but the formatting check wasn't printing which files need to be reformatted, so I have made a PR to fix that. |
|
@owen-mc I'm so sorry for many mistakes! |
|
No problem. There is one compile error left for Beego:
|
go/ql/test/library-tests/semmle/go/frameworks/Beego/CleartextLogging.expected
Outdated
Show resolved
Hide resolved
I added new important System File Access Sinks.
I didn't know how to complete the Library Tests.
Also I put all the things in one query file, I knew that there was frameworks for some of these new sinks but I wanted to keep this pull request simple for review, I think It'll be better if I wait for your suggestion about move each new sink to a proper location.
Please to Skip writing your own local tests refer to following: https://github.com/amammad/afero_And_WebServers